Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Messaging. FIX: issue with removeToken on ios. #1754

Closed
wants to merge 1 commit into from
Closed

Messaging. FIX: issue with removeToken on ios. #1754

wants to merge 1 commit into from

Conversation

kholiavko-roman
Copy link

Its related to this issue #1510

Summary

Currently on iOS if we remove token by firebase.messaging().deleteToken(); and try to get new one after that - it returns previous.

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in /tests/e2e/*
  • Updated the documentation in the docs repo
    • LINK TO DOCS PR HERE
  • Flow types updated
  • Typescript types updated

Test Plan

const oldToken = await firebase.messaging().getToken();
await firebase.messaging().deleteToken();
const newToken = await firebase.messaging().getToken();
if (oldToken === newToken) {
    console.error('Token has not been refreshed');
} else {
     console.log('Token has refreshed successfully!');
}

This code must output Token has refreshed successfully!'

Release Plan

[IOS][BUGFIX] [MESSAGING] - fixed bug with removeToken on ios.

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #1754 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1754   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

1 similar comment
@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #1754 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1754   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

@codecov
Copy link

codecov bot commented Dec 12, 2018 •
edited
Loading

Codecov Report

Merging #1754 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1754   +/-   ##
=======================================
  Coverage   76.16%   76.16%           
=======================================
  Files          72       72           
  Lines        1838     1838           
=======================================
  Hits         1400     1400           
  Misses        438      438

@kholiavko-roman kholiavko-roman changed the title FIX: issue with removeToken on ios. Messaging. FIX: issue with removeToken on ios. Dec 14, 2018
@Salakar Salakar mentioned this pull request Jan 3, 2019
Merged
6 tasks
@Salakar
Copy link
Member

Salakar commented Jan 3, 2019 •
edited
Loading

Hey, I fixed this as part of #1805 and also added tests to confirm - thanks for the heads up and for this contribution 🔥 this will land in v5.2.0.

Tests: https://github.com/invertase/react-native-firebase/blob/master/tests/e2e/messaging/messaging.e2e.js

Loving react-native-firebase and the support we provide? Please consider supporting us with any of the below:

@Salakar Salakar closed this Jan 3, 2019
@Salakar Salakar self-assigned this Jan 3, 2019
@Salakar Salakar added this to the v5.2.0 milestone Jan 3, 2019
@Salakar Salakar added platform: ios plugin: messaging FCM only - ( messaging() ) - do not use for Notifications labels Jan 3, 2019
@marqroldan
Copy link

Hi @Salakar, sorry for tagging but it seems like this issue is present in version 6.7.1

The test for this one is also not present in v6. Should I open a ticket? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Salakar Salakar

Labels
platform: ios plugin: messaging FCM only - ( messaging() ) - do not use for Notifications resolution: duplicate
Projects
None yet
Milestone
v5.2.x
Development

Successfully merging this pull request may close these issues.
4 participants
@kholiavko-roman @CLAassistant @Salakar @marqroldan